feat: Full support for rendering blocks in inventory GUI powered by deeplsate #292
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThis pull request updates the dependency version in the package configuration, introduces a new file for GUI rendering in a 3D environment, and augments the asset update process with asynchronous GUI texture generation. It also refines inventory window functions to allow optional image parameters, adapting texture retrieval logic and removing an external render method. The changes adjust both control flows for GUI atlas generation and slot rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant WR as WorldRendererCommon
participant GR as GuiRenderer
participant AP as AssetsParser
participant Canvas as WebGL2 Canvas
WR->>WR: updateAssetsData()
WR->>GR: generateGuiTextures()
GR->>GR: generateGuiAtlas()
GR->>GR: getNonFullBlocksModels()
GR->>AP: Resolve block models & item definitions
GR->>Canvas: generateItemsGui()
GR->>GR: generateAtlas()
GR-->>WR: Return compiled GUI atlas
WR->>WR: Complete assets update with atlas
sequenceDiagram
participant RI as renderSlot
participant GA as activeGuiAtlas
participant GI as getImage
GI->>RI: Called with { path, texture, blockData, image? }
alt Image provided
RI-->>RI: Return provided image immediately
else No image provided
RI->>RI: Check fullBlockModelSupport flag
alt fullBlockModelSupport disabled
RI->>GA: Retrieve texture details from atlas
RI-->>RI: Return texture data with atlas image
else fullBlockModelSupport enabled
RI-->>RI: Proceed with standard processing
end
end
Suggested labels
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
|
/deploy |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
renderer/viewer/lib/worldrendererCommon.ts (3)
358-360: Consider adding error handling.
WrappinggenerateGuiAtlas()in a try/catch block might help detect or log failures, preventing silent errors within async flows.
386-386: Evaluate potential performance overhead oftoDataURL().
Usingcanvas.toDataURL()can be memory-intensive. Consider alternatives likecanvas.toBlob()or object URLs for incremental performance and memory gains.
429-431: Avoid console timing in production builds.
While console timing is useful for debugging, consider gating it behind a debug flag or removing it before production to keep logs clean.renderer/viewer/lib/guiRenderer.ts (3)
1-14: Validate usage of global references.
Imports and placeholders suggest reliance on a globalviewer. Passing it as a parameter could improve testability and maintainability.
16-106: Refine global dependency ingetNonFullBlocksModels().
Callingviewer.worldwithin this function might impair reusability. Consider injecting world data orviewerexplicitly for better modular design.
248-261: Optional error handling ingenerateGuiAtlas().
Merging block and item images, then building the combined atlas relies on no errors ingenerateItemsGui. For safer workflows, consider a try/catch or thorough checks to handle exceptions.src/inventoryWindows.ts (1)
158-160: Optional parameter prioritization.
Returning early whenimageis provided is concise. IfgetImagegrows more specialized, consider clarifying the order of preference in docs or function name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(1 hunks)renderer/viewer/lib/guiRenderer.ts(1 hunks)renderer/viewer/lib/worldrendererCommon.ts(4 hunks)src/inventoryWindows.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (6)
renderer/viewer/lib/worldrendererCommon.ts (1)
27-27: New import looks correct.
No issues with importinggenerateGuiAtlas. The usage is consistent within this file.renderer/viewer/lib/guiRenderer.ts (3)
117-221: Approve fallback for missing textures.
Your approach of substitutingblock/unknownfor faces with missing textures and warning about them is robust for debugging. Nice job ensuring partial coverage of erroneous states.
223-246: Atlas creation logic looks solid.
ThegenerateAtlasfunction effectively compiles images and updatesactiveGuiAtlas. Consider verifying final atlas dimensions (especially for large sets) to avoid layout overflows.
262-270: Usage example is clear.
The commented-out example indicates how to integrate with user-defined model data. Good for documentation and testing.src/inventoryWindows.ts (2)
13-13: New import reference.
UsingactiveGuiAtlasaligns well with the updated GUI texture pipelines introduced inguiRenderer.ts.
187-188: Extended return properties enhance slot rendering.
AddingmodelNameandimageto the returned data creates flexibility for custom or advanced rendering. This looks well integrated with the rest of the logic.
|
Deployed to Vercel Preview: https://prismarine-mmbrp0s8h-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
renderer/viewer/lib/guiRenderer.ts (6)
47-85: Hard-coded display values could be extracted as constants.The standard GUI display values are hard-coded within the function, which makes maintenance difficult and could lead to inconsistencies if used elsewhere in the codebase.
Extract these values as named constants:
+ // Standard GUI display configuration + const STANDARD_GUI_DISPLAY = { + 'rotation': [30, 225, 0], + 'translation': [0, 0, 0], + 'scale': [0.625, 0.625, 0.625] + } if (resolvedModel?.elements) { - const standardGuiDisplay = { - 'rotation': [ - 30, - 225, - 0 - ], - 'translation': [ - 0, - 0, - 0 - ], - 'scale': [ - 0.625, - 0.625, - 0.625 - ] - } + const standardGuiDisplay = STANDARD_GUI_DISPLAY
108-110: Remove commented-out code.Commented-out code adds noise and can confuse future developers. If this is meant for debugging, consider adding it as a proper debug function that can be enabled via a flag.
Either remove the commented-out code or convert it to a proper debug function:
- // customEvents.on('gameLoaded', () => { - // const res = getNonFullBlocksModels() - // }) + // Debug helper that can be enabled in development + const DEBUG = false + if (DEBUG) { + customEvents.on('gameLoaded', () => { + console.debug('Non-full blocks models:', getNonFullBlocksModels()) + }) + }
178-179: Remove debugging code from production.The commented-out
includeOnlyarray and its empty initialization suggest debugging code that was left in the production version.Either properly implement this as a configurable feature or remove it entirely:
- // const includeOnly = ['powered_repeater', 'wooden_door'] - const includeOnly = [] as string[] + // Optional filtering for debugging or testing + const config = { + includeOnly: [] as string[] + }Then use
config.includeOnlyin your code.
186-211: Consider batch processing for better performance.The loop processes each model sequentially, which might not be optimal for performance, especially when dealing with many models.
Consider batching the model processing or using a worker thread for rendering to improve performance, especially on slower devices.
231-235: Remove commented-out debugging code.This code for generating and downloading debug images should be moved to a dedicated debug utility function rather than being commented out in the production code.
Either remove the code or move it to a proper debug utility:
- // const atlasParser = new AtlasParser({ latest: atlas.json }, atlas.canvas.toDataURL()) - // const a = document.createElement('a') - // a.href = await atlasParser.createDebugImage(true) - // a.download = 'blocks_atlas.png' - // a.click() + // Debug utility for atlas visualization + const saveAtlasDebugImage = async (atlas) => { + if (!DEBUG) return + + const atlasParser = new AtlasParser({ latest: atlas.json }, atlas.canvas.toDataURL()) + const a = document.createElement('a') + a.href = await atlasParser.createDebugImage(true) + a.download = 'blocks_atlas.png' + a.click() + }
1-257: Add type definitions for better code quality.Many functions and variables lack explicit TypeScript type definitions, which can lead to potential runtime errors and makes the code harder to maintain.
Add explicit type definitions to function parameters, return types, and variables:
- const generateItemsGui = async (models: Record<string, BlockModelMcAssets>, isItems = false) => { + const generateItemsGui = async ( + models: Record<string, BlockModelMcAssets>, + isItems = false + ): Promise<Record<string, HTMLImageElement>> => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
renderer/viewer/lib/guiRenderer.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (2)
renderer/viewer/lib/guiRenderer.ts (2)
12-14: Good use of Valtio proxy for reactive state management.The
activeGuiAtlasproxy object allows components to react to texture atlas changes, which is a good design pattern for managing shared state.
245-257: Good orchestration function with performance tracking.The
generateGuiAtlasfunction effectively orchestrates the atlas generation process and includes performance tracking withconsole.time, which is useful for debugging and optimization.However, there's commented-out code on line 256 that should be removed or properly implemented.
| // Import placeholders - replace with actual imports for your environment | ||
| import { ItemRenderer, Identifier, ItemStack, NbtString, Structure, StructureRenderer, ItemRendererResources, BlockDefinition, BlockModel, TextureAtlas, Resources, ItemModel } from 'deepslate' | ||
| import { mat4, vec3 } from 'gl-matrix' | ||
| import { AssetsParser } from 'mc-assets/dist/assetsParser' | ||
| import { getLoadedImage } from 'mc-assets/dist/utils' | ||
| import { BlockModel as BlockModelMcAssets, AtlasParser } from 'mc-assets' | ||
| import { getLoadedBlockstatesStore, getLoadedModelsStore } from 'mc-assets/dist/stores' | ||
| import { makeTextureAtlas } from 'mc-assets/dist/atlasCreator' | ||
| import { proxy, ref } from 'valtio' | ||
| import { getItemDefinition } from 'mc-assets/dist/itemDefinitions' | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider replacing placeholder imports with actual imports.
The comment on line 1 suggests these might be placeholder imports. Ensure all imports are correctly used and necessary for the implementation.
🏁 Script executed:
#!/bin/bash
# Check if all imports are actually used
rg -A 2 "import \{[^}]*" renderer/viewer/lib/guiRenderer.ts | grep -o '{[^}]*}' | tr ',' '\n' | grep -v "{" | sed 's/{//g' | sed 's/}//g' | tr -d ' ' > imports.txt
for import in $(cat imports.txt); do
count=$(rg "\b$import\b" renderer/viewer/lib/guiRenderer.ts | wc -l)
if [ $count -le 1 ]; then
echo "Potentially unused import: $import (found $count times)"
fi
done
rm imports.txtLength of output: 2619
Action Required: Update Placeholder and Unused Imports in guiRenderer.ts
In renderer/viewer/lib/guiRenderer.ts (lines 1–11), please ensure that the placeholder imports are replaced with the actual, environment-specific implementations. Additionally, our analysis indicates that some imported names appear unused:
- Structure (referenced only once)
- StructureRenderer (referenced only once)
- Resources (referenced only once)
- vec3 (referenced only once)
Review these imports to decide whether they’re needed for future functionality or should be removed to maintain a clean codebase.
| if (!gl) { | ||
| throw new Error('Cannot get WebGL2 context') | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling lacks fallback.
The error handling for WebGL2 context acquisition throws an error without providing fallback behavior, which could cause the application to crash on unsupported browsers.
Consider adding a fallback or graceful degradation:
const gl = canvas.getContext('webgl2', { preserveDrawingBuffer: true })
if (!gl) {
- throw new Error('Cannot get WebGL2 context')
+ console.error('WebGL2 not supported, trying fallback to WebGL')
+ const fallbackGl = canvas.getContext('webgl', { preserveDrawingBuffer: true })
+ if (!fallbackGl) {
+ console.error('WebGL not supported, using software rendering fallback')
+ // Implement software fallback or show warning to user
+ throw new Error('WebGL not supported in this browser')
+ }
+ return generateFallbackItemsGui(models, isItems, fallbackGl)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!gl) { | |
| throw new Error('Cannot get WebGL2 context') | |
| } | |
| const gl = canvas.getContext('webgl2', { preserveDrawingBuffer: true }) | |
| if (!gl) { | |
| console.error('WebGL2 not supported, trying fallback to WebGL') | |
| const fallbackGl = canvas.getContext('webgl', { preserveDrawingBuffer: true }) | |
| if (!fallbackGl) { | |
| console.error('WebGL not supported, using software rendering fallback') | |
| // Implement software fallback or show warning to user | |
| throw new Error('WebGL not supported in this browser') | |
| } | |
| return generateFallbackItemsGui(models, isItems, fallbackGl) | |
| } |
renderer/viewer/lib/guiRenderer.ts
Outdated
| canvasTemp.style.imageRendering = 'pixelated' | ||
| const ctx = canvasTemp.getContext('2d')! | ||
| ctx.imageSmoothingEnabled = false | ||
| ctx.drawImage(img, 0, 0) | ||
|
|
||
| const atlasParser = isItems ? viewer.world.itemsAtlasParser! : viewer.world.blocksAtlasParser! | ||
| const textureAtlas = new TextureAtlas( | ||
| ctx.getImageData(0, 0, img.width, img.height), | ||
| Object.fromEntries(Object.entries(atlasParser.atlas.latest.textures).map(([key, value]) => { | ||
| return [key, [ | ||
| value.u, | ||
| value.v, | ||
| (value.u + (value.su ?? atlasParser.atlas.latest.suSv)), | ||
| (value.v + (value.sv ?? atlasParser.atlas.latest.suSv)), | ||
| ]] as [string, [number, number, number, number]] | ||
| })) | ||
| ) | ||
|
|
||
| const PREVIEW_ID = Identifier.parse('preview:preview') | ||
| const PREVIEW_DEFINITION = new BlockDefinition({ '': { model: PREVIEW_ID.toString() } }, undefined) | ||
|
|
||
| let modelData: any | ||
| const resources: ItemRendererResources = { | ||
| getBlockModel (id) { | ||
| if (id.equals(PREVIEW_ID)) { | ||
| return BlockModel.fromJson(modelData ?? {}) | ||
| } | ||
| return null | ||
| }, | ||
| getTextureUV (texture) { | ||
| return textureAtlas.getTextureUV(texture.toString().split('/')[1]! as any) | ||
| }, | ||
| getTextureAtlas () { | ||
| return textureAtlas.getTextureAtlas() | ||
| }, | ||
| getItemComponents (id) { | ||
| return new Map() | ||
| }, | ||
| getItemModel (id) { | ||
| if (id.equals(PREVIEW_ID)) { | ||
| return ItemModel.fromJson({ type: 'minecraft:model', model: PREVIEW_ID.toString() }) | ||
| } | ||
| return null | ||
| }, | ||
| } | ||
|
|
||
| const canvas = document.createElement('canvas') | ||
| canvas.width = RENDER_SIZE | ||
| canvas.height = RENDER_SIZE | ||
| const gl = canvas.getContext('webgl2', { preserveDrawingBuffer: true }) | ||
| if (!gl) { | ||
| throw new Error('Cannot get WebGL2 context') | ||
| } | ||
|
|
||
| function resetGLContext (gl) { | ||
| gl.clearColor(0, 0, 0, 0) | ||
| gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT | gl.STENCIL_BUFFER_BIT) | ||
| } | ||
|
|
||
| // const includeOnly = ['powered_repeater', 'wooden_door'] | ||
| const includeOnly = [] as string[] | ||
|
|
||
| const images: Record<string, HTMLImageElement> = {} | ||
| const item = new ItemStack(PREVIEW_ID, 1, new Map(Object.entries({ | ||
| 'minecraft:item_model': new NbtString(PREVIEW_ID.toString()), | ||
| }))) | ||
| const renderer = new ItemRenderer(gl, item, resources, { display_context: 'gui' }) | ||
| const missingTextures = new Set() | ||
| for (const [modelName, model] of Object.entries(models)) { | ||
| if (includeOnly.length && !includeOnly.includes(modelName)) continue | ||
|
|
||
| const patchMissingTextures = () => { | ||
| for (const element of model.elements ?? []) { | ||
| for (const [faceName, face] of Object.entries(element.faces)) { | ||
| if (face.texture.startsWith('#')) { | ||
| missingTextures.add(`${modelName} ${faceName}: ${face.texture}`) | ||
| face.texture = 'block/unknown' | ||
| } | ||
| } | ||
| } | ||
| } | ||
| patchMissingTextures() | ||
|
|
||
| modelData = model | ||
| resetGLContext(gl) | ||
| if (!modelData) continue | ||
| renderer.setItem(item, { display_context: 'gui' }) | ||
| renderer.drawItem() | ||
| const url = canvas.toDataURL() | ||
| // eslint-disable-next-line no-await-in-loop | ||
| const img = await getLoadedImage(url) | ||
| images[modelName] = img | ||
| } | ||
|
|
||
| if (missingTextures.size) { | ||
| console.warn(`[guiRenderer] Missing textures in ${[...missingTextures].join(', ')}`) | ||
| } | ||
|
|
||
| return images | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Long function with multiple responsibilities needs refactoring.
This function is doing too much: setting up WebGL context, creating texture atlas, rendering items, handling missing textures, and generating images. It should be split into smaller, focused functions.
Consider breaking this function into smaller ones:
- A function to set up the WebGL context
- A function to create resources
- A function to handle missing textures
- A function to render a single item
- The main coordinating function
Also, the eslint-disable comment on line 208 indicates a potential performance issue with awaiting in a loop. Consider using Promise.all to process all images in parallel:
- // eslint-disable-next-line no-await-in-loop
- const img = await getLoadedImage(url)
- images[modelName] = img
+ const imagePromises = []
+ const modelNames = []
+
+ // Queue all image loading requests
+ for (const [modelName, model] of Object.entries(models)) {
+ // ... existing code ...
+ const url = canvas.toDataURL()
+ imagePromises.push(getLoadedImage(url))
+ modelNames.push(modelName)
+ }
+
+ // Wait for all images to load in parallel
+ const loadedImages = await Promise.all(imagePromises)
+
+ // Assign loaded images to the results object
+ for (let i = 0; i < modelNames.length; i++) {
+ images[modelNames[i]] = loadedImages[i]
+ }Committable suggestion skipped: line range outside the PR's diff.
renderer/viewer/lib/guiRenderer.ts
Outdated
| export const getNonFullBlocksModels = () => { | ||
| const version = viewer.world.texturesVersion ?? 'latest' | ||
| const itemsDefinitions = viewer.world.itemsDefinitionsStore.data.latest | ||
| const blockModelsResolved = {} as Record<string, any> | ||
| const itemsModelsResolved = {} as Record<string, any> | ||
| const fullBlocksWithNonStandardDisplay = [] as string[] | ||
| const handledItemsWithDefinitions = new Set() | ||
| const assetsParser = new AssetsParser(version, getLoadedBlockstatesStore(viewer.world.blockstatesModels), getLoadedModelsStore(viewer.world.blockstatesModels)) | ||
|
|
||
| const arrEqual = (a: number[], b: number[]) => a.length === b.length && a.every((x, i) => x === b[i]) | ||
| const addModelIfNotFullblock = (name: string, model: BlockModelMcAssets) => { | ||
| if (blockModelsResolved[name]) return | ||
| if (!model?.elements?.length) return | ||
| const isFullBlock = model.elements.length === 1 && arrEqual(model.elements[0].from, [0, 0, 0]) && arrEqual(model.elements[0].to, [16, 16, 16]) | ||
| if (isFullBlock) return | ||
| blockModelsResolved[name] = model | ||
| } | ||
|
|
||
| for (const [name, definition] of Object.entries(itemsDefinitions)) { | ||
| const item = getItemDefinition(viewer.world.itemsDefinitionsStore, { | ||
| version, | ||
| name, | ||
| properties: { | ||
| 'minecraft:display_context': 'gui', | ||
| }, | ||
| }) | ||
| if (item) { | ||
| const { resolvedModel } = assetsParser.getResolvedModelsByModel((item.special ? name : item.model).replace('minecraft:', '')) ?? {} | ||
| if (resolvedModel) { | ||
| handledItemsWithDefinitions.add(name) | ||
| } | ||
| if (resolvedModel?.elements) { | ||
| const standardGuiDisplay = { | ||
| 'rotation': [ | ||
| 30, | ||
| 225, | ||
| 0 | ||
| ], | ||
| 'translation': [ | ||
| 0, | ||
| 0, | ||
| 0 | ||
| ], | ||
| 'scale': [ | ||
| 0.625, | ||
| 0.625, | ||
| 0.625 | ||
| ] | ||
| } | ||
| let hasStandardDisplay = true | ||
| if (resolvedModel['display']?.gui) { | ||
| hasStandardDisplay = | ||
| arrEqual(resolvedModel['display'].gui.rotation, standardGuiDisplay.rotation) | ||
| && arrEqual(resolvedModel['display'].gui.translation, standardGuiDisplay.translation) | ||
| && arrEqual(resolvedModel['display'].gui.scale, standardGuiDisplay.scale) | ||
| } else { | ||
| resolvedModel['display'] ??= {} | ||
| resolvedModel['display']['gui'] = standardGuiDisplay | ||
| } | ||
|
|
||
| addModelIfNotFullblock(name, resolvedModel) | ||
|
|
||
| if (!blockModelsResolved[name] && !hasStandardDisplay) { | ||
| fullBlocksWithNonStandardDisplay.push(name) | ||
| } | ||
| const notSideLight = resolvedModel['gui_light'] !== 'side' | ||
| if (!hasStandardDisplay || notSideLight) { | ||
| blockModelsResolved[name] = resolvedModel | ||
| } | ||
| } | ||
| if (!blockModelsResolved[name] && item.tints && resolvedModel) { | ||
| itemsModelsResolved[name] = resolvedModel | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const [name, blockstate] of Object.entries(viewer.world.blockstatesModels.blockstates.latest)) { | ||
| if (handledItemsWithDefinitions.has(name)) { | ||
| continue | ||
| } | ||
| const resolvedModel = assetsParser.getResolvedModelFirst({ name: name.replace('minecraft:', ''), properties: {} }, true) | ||
| if (resolvedModel) { | ||
| addModelIfNotFullblock(name, resolvedModel[0]) | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| blockModelsResolved, | ||
| itemsModelsResolved | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function is too complex and has undefined global dependencies.
This function has several issues:
- It references undefined global variables (
viewer.world) - It's overly complex (90 lines) and should be broken down into smaller focused functions
- It has multiple responsibilities (filtering blocks, processing item definitions, handling display properties)
Consider refactoring this function into smaller, focused functions:
- export const getNonFullBlocksModels = () => {
+ // Get the current version from the world renderer
+ const getCurrentVersion = () => viewer.world.texturesVersion ?? 'latest'
+
+ // Check if arrays have the same values
+ const arrEqual = (a: number[], b: number[]) => a.length === b.length && a.every((x, i) => x === b[i])
+
+ // Check if a block model represents a full block
+ const isFullBlock = (model: BlockModelMcAssets) => {
+ return model.elements?.length === 1 &&
+ arrEqual(model.elements[0].from, [0, 0, 0]) &&
+ arrEqual(model.elements[0].to, [16, 16, 16])
+ }
+
+ // Process a block model and add it if not a full block
+ const processBlockModel = (name: string, model: BlockModelMcAssets, blockModelsResolved: Record<string, any>) => {
+ if (blockModelsResolved[name] || !model?.elements?.length) return false
+
+ if (isFullBlock(model)) return false
+
+ blockModelsResolved[name] = model
+ return true
+ }
+
+ export const getNonFullBlocksModels = () => {Also, consider adding proper error handling if viewer is undefined or doesn't have the expected properties.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getNonFullBlocksModels = () => { | |
| const version = viewer.world.texturesVersion ?? 'latest' | |
| const itemsDefinitions = viewer.world.itemsDefinitionsStore.data.latest | |
| const blockModelsResolved = {} as Record<string, any> | |
| const itemsModelsResolved = {} as Record<string, any> | |
| const fullBlocksWithNonStandardDisplay = [] as string[] | |
| const handledItemsWithDefinitions = new Set() | |
| const assetsParser = new AssetsParser(version, getLoadedBlockstatesStore(viewer.world.blockstatesModels), getLoadedModelsStore(viewer.world.blockstatesModels)) | |
| const arrEqual = (a: number[], b: number[]) => a.length === b.length && a.every((x, i) => x === b[i]) | |
| const addModelIfNotFullblock = (name: string, model: BlockModelMcAssets) => { | |
| if (blockModelsResolved[name]) return | |
| if (!model?.elements?.length) return | |
| const isFullBlock = model.elements.length === 1 && arrEqual(model.elements[0].from, [0, 0, 0]) && arrEqual(model.elements[0].to, [16, 16, 16]) | |
| if (isFullBlock) return | |
| blockModelsResolved[name] = model | |
| } | |
| for (const [name, definition] of Object.entries(itemsDefinitions)) { | |
| const item = getItemDefinition(viewer.world.itemsDefinitionsStore, { | |
| version, | |
| name, | |
| properties: { | |
| 'minecraft:display_context': 'gui', | |
| }, | |
| }) | |
| if (item) { | |
| const { resolvedModel } = assetsParser.getResolvedModelsByModel((item.special ? name : item.model).replace('minecraft:', '')) ?? {} | |
| if (resolvedModel) { | |
| handledItemsWithDefinitions.add(name) | |
| } | |
| if (resolvedModel?.elements) { | |
| const standardGuiDisplay = { | |
| 'rotation': [ | |
| 30, | |
| 225, | |
| 0 | |
| ], | |
| 'translation': [ | |
| 0, | |
| 0, | |
| 0 | |
| ], | |
| 'scale': [ | |
| 0.625, | |
| 0.625, | |
| 0.625 | |
| ] | |
| } | |
| let hasStandardDisplay = true | |
| if (resolvedModel['display']?.gui) { | |
| hasStandardDisplay = | |
| arrEqual(resolvedModel['display'].gui.rotation, standardGuiDisplay.rotation) | |
| && arrEqual(resolvedModel['display'].gui.translation, standardGuiDisplay.translation) | |
| && arrEqual(resolvedModel['display'].gui.scale, standardGuiDisplay.scale) | |
| } else { | |
| resolvedModel['display'] ??= {} | |
| resolvedModel['display']['gui'] = standardGuiDisplay | |
| } | |
| addModelIfNotFullblock(name, resolvedModel) | |
| if (!blockModelsResolved[name] && !hasStandardDisplay) { | |
| fullBlocksWithNonStandardDisplay.push(name) | |
| } | |
| const notSideLight = resolvedModel['gui_light'] !== 'side' | |
| if (!hasStandardDisplay || notSideLight) { | |
| blockModelsResolved[name] = resolvedModel | |
| } | |
| } | |
| if (!blockModelsResolved[name] && item.tints && resolvedModel) { | |
| itemsModelsResolved[name] = resolvedModel | |
| } | |
| } | |
| } | |
| for (const [name, blockstate] of Object.entries(viewer.world.blockstatesModels.blockstates.latest)) { | |
| if (handledItemsWithDefinitions.has(name)) { | |
| continue | |
| } | |
| const resolvedModel = assetsParser.getResolvedModelFirst({ name: name.replace('minecraft:', ''), properties: {} }, true) | |
| if (resolvedModel) { | |
| addModelIfNotFullblock(name, resolvedModel[0]) | |
| } | |
| } | |
| return { | |
| blockModelsResolved, | |
| itemsModelsResolved | |
| } | |
| } | |
| // Get the current version from the world renderer | |
| const getCurrentVersion = () => viewer.world.texturesVersion ?? 'latest' | |
| // Check if arrays have the same values | |
| const arrEqual = (a: number[], b: number[]) => | |
| a.length === b.length && a.every((x, i) => x === b[i]) | |
| // Check if a block model represents a full block | |
| const isFullBlock = (model: BlockModelMcAssets) => { | |
| return ( | |
| model.elements?.length === 1 && | |
| arrEqual(model.elements[0].from, [0, 0, 0]) && | |
| arrEqual(model.elements[0].to, [16, 16, 16]) | |
| ) | |
| } | |
| // Process a block model and add it if not a full block | |
| const processBlockModel = ( | |
| name: string, | |
| model: BlockModelMcAssets, | |
| blockModelsResolved: Record<string, any> | |
| ) => { | |
| if (blockModelsResolved[name] || !model?.elements?.length) return false | |
| if (isFullBlock(model)) return false | |
| blockModelsResolved[name] = model | |
| return true | |
| } | |
| export const getNonFullBlocksModels = () => { | |
| const version = getCurrentVersion() | |
| const itemsDefinitions = viewer.world.itemsDefinitionsStore.data.latest | |
| const blockModelsResolved = {} as Record<string, any> | |
| const itemsModelsResolved = {} as Record<string, any> | |
| const fullBlocksWithNonStandardDisplay = [] as string[] | |
| const handledItemsWithDefinitions = new Set() | |
| const assetsParser = new AssetsParser( | |
| version, | |
| getLoadedBlockstatesStore(viewer.world.blockstatesModels), | |
| getLoadedModelsStore(viewer.world.blockstatesModels) | |
| ) | |
| // TODO: Eventually replace the inline logic with the helper "processBlockModel" | |
| const addModelIfNotFullblock = (name: string, model: BlockModelMcAssets) => { | |
| if (blockModelsResolved[name]) return | |
| if (!model?.elements?.length) return | |
| if (isFullBlock(model)) return | |
| blockModelsResolved[name] = model | |
| } | |
| for (const [name, definition] of Object.entries(itemsDefinitions)) { | |
| const item = getItemDefinition(viewer.world.itemsDefinitionsStore, { | |
| version, | |
| name, | |
| properties: { | |
| 'minecraft:display_context': 'gui', | |
| }, | |
| }) | |
| if (item) { | |
| const { resolvedModel } = | |
| assetsParser.getResolvedModelsByModel( | |
| (item.special ? name : item.model).replace('minecraft:', '') | |
| ) ?? {} | |
| if (resolvedModel) { | |
| handledItemsWithDefinitions.add(name) | |
| } | |
| if (resolvedModel?.elements) { | |
| const standardGuiDisplay = { | |
| rotation: [30, 225, 0], | |
| translation: [0, 0, 0], | |
| scale: [0.625, 0.625, 0.625], | |
| } | |
| let hasStandardDisplay = true | |
| if (resolvedModel['display']?.gui) { | |
| hasStandardDisplay = | |
| arrEqual(resolvedModel['display'].gui.rotation, standardGuiDisplay.rotation) && | |
| arrEqual(resolvedModel['display'].gui.translation, standardGuiDisplay.translation) && | |
| arrEqual(resolvedModel['display'].gui.scale, standardGuiDisplay.scale) | |
| } else { | |
| resolvedModel['display'] ??= {} | |
| resolvedModel['display']['gui'] = standardGuiDisplay | |
| } | |
| addModelIfNotFullblock(name, resolvedModel) | |
| if (!blockModelsResolved[name] && !hasStandardDisplay) { | |
| fullBlocksWithNonStandardDisplay.push(name) | |
| } | |
| const notSideLight = resolvedModel['gui_light'] !== 'side' | |
| if (!hasStandardDisplay || notSideLight) { | |
| blockModelsResolved[name] = resolvedModel | |
| } | |
| } | |
| if (!blockModelsResolved[name] && item.tints && resolvedModel) { | |
| itemsModelsResolved[name] = resolvedModel | |
| } | |
| } | |
| } | |
| for (const [name, blockstate] of Object.entries(viewer.world.blockstatesModels.blockstates.latest)) { | |
| if (handledItemsWithDefinitions.has(name)) { | |
| continue | |
| } | |
| const resolvedModel = assetsParser.getResolvedModelFirst( | |
| { name: name.replace('minecraft:', ''), properties: {} }, | |
| true | |
| ) | |
| if (resolvedModel) { | |
| addModelIfNotFullblock(name, resolvedModel[0]) | |
| } | |
| } | |
| // Consider adding error handling if 'viewer' or its properties are undefined | |
| return { | |
| blockModelsResolved, | |
| itemsModelsResolved, | |
| } | |
| } |
There was a problem hiding this comment.
@zardoy I'm glad you found the suggestion helpful! Would you like me to help implement the refactoring changes? The suggested approach would make the code more maintainable by:
- Breaking down the complex function into smaller, focused helper functions
- Improving readability with explicit function names that describe what each part does
- Making it easier to test individual parts of the logic
Let me know if you'd like assistance with implementing these changes or if you have any questions about the suggested approach. I can help with the full implementation whenever you're ready.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/react/EnterFullscreenButton.tsx (1)
37-43: Great improvement to error handling for fullscreen requests.Converting to an async/await pattern with proper error handling is excellent. The fullscreen API can fail for various reasons (browser restrictions, not being triggered by a user event, etc.), and this change ensures users get appropriate feedback rather than silent failures.
Consider adding more specific error handling for common fullscreen API errors:
try { await document.documentElement.requestFullscreen() } catch (err) { - showNotification(`${err.message ?? err}`, undefined, true) + // Handle specific error types with more user-friendly messages + if (err.name === 'NotAllowedError') { + showNotification('Fullscreen request was denied. This may require user interaction.', undefined, true) + } else if (err.name === 'NotSupportedError') { + showNotification('Fullscreen is not supported by your browser.', undefined, true) + } else { + showNotification(`${err.message ?? err}`, undefined, true) + } }src/inventoryWindows.ts (1)
208-208: Consider refactoring the complex string replacement chain.The current approach uses multiple consecutive replace operations which can be hard to maintain and understand.
Consider using a more maintainable approach with a regular expression or a standardized helper function:
- const item = atlas?.textures[itemModelName.replace('minecraft:', '').replace('block/', '').replace('blocks/', '').replace('item/', '').replace('items/', '')] + // Define a helper function elsewhere in the codebase + const normalizeTextureKey = (name: string) => { + return name.replace(/^minecraft:/, '') + .replace(/^(blocks?|items?)\//, ''); + } + const item = atlas?.textures[normalizeTextureKey(itemModelName)]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
renderer/viewer/lib/guiRenderer.ts(1 hunks)src/inventoryWindows.ts(7 hunks)src/react/EnterFullscreenButton.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- renderer/viewer/lib/guiRenderer.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (7)
src/react/EnterFullscreenButton.tsx (1)
7-7: Well-structured import for notification functionality.Adding the notification capability improves error handling for fullscreen requests, which is a good practice since fullscreen requests can fail due to browser restrictions or user settings.
src/inventoryWindows.ts (6)
13-13: Good addition of GUI atlas import.This import connects to the new GUI rendering system mentioned in the PR description, enabling texture atlas-based rendering of inventory items.
159-162: Enhanced flexibility with optional image parameter.Adding an optional image parameter to the getImage function improves flexibility by allowing direct image passing, potentially skipping unnecessary loading steps and improving performance.
186-193: Good enhancement to renderSlot function signature.The addition of modelName and image optional properties to the return type enhances type safety and provides more information for rendering.
263-263: Simplified debugIsQuickbar assignment.The removal of unnecessary parentheses around the ternary operation is a good clean-up.
332-332: Straightforward code simplification.Removing unnecessary variable declaration and directly passing the value to mapSlots improves code readability.
205-219:❓ Verification inconclusive
Verify the atlas lookup implementation for efficiency.
The implementation looks good overall, but there are a few concerns:
- There's a TODO comment indicating this approach might not be optimal
- The string replacement chain is complex and might be fragile
Consider evaluating if this string manipulation approach is the most robust way to handle texture lookups:
🏁 Script executed:
#!/bin/bash # Find similar string manipulation patterns in the codebase to determine consistency rg -A 2 -B 2 "replace\\('minecraft:'" --glob "*.ts" --glob "*.tsx" # Check if there are helper functions that could be used instead ast-grep --pattern 'function $name($$$) { $$$ return $$$replace($$$) $$$ }'Length of output: 2123
Atlas Lookup & String Normalization Evaluation
The atlas lookup code in
src/inventoryWindows.ts(lines 205–219) uses a chain of string replacements to normalize texture names. Our verification confirms that this pattern appears consistently across the codebase (e.g., inrenderer/viewer/lib/guiRenderer.ts,src/sounds/botSoundSystem.ts, andsrc/react/MessageFormatted.tsx). Although the implementation appears to be efficient for its current purpose, the repeated string manipulation is somewhat fragile and may be harder to maintain.
- Consider extracting this string normalization logic into a helper function for better maintainability and consistency.
- Revisit the TODO comment regarding atlas model fetching to evaluate if a lazy-loading or on-demand approach might be more robust.
- Verify that the current chain does not introduce performance bottlenecks when used at scale.
|
|
||
| if (!fullBlockModelSupport) { | ||
| const atlas = activeGuiAtlas.atlas?.json | ||
| // todo atlas holds all rendered blocks, not all possibly rendered item/block models, need to request this on demand instead (this is how vanilla works) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
TODO comment needs to be addressed.
The TODO comment indicates a potential architectural issue with how the atlas handles rendered blocks vs. all possible item/block models.
Is this TODO being tracked in an issue? Let's see if there are similar TODOs in the codebase:
🏁 Script executed:
#!/bin/bash
# Look for similar atlas-related TODOs in the codebase
rg "todo.*atlas" --glob "*.ts" --glob "*.tsx"
# Check if there's a GitHub issue tracking this
gh issue list --search "atlas holds all rendered blocks"Length of output: 287
Action Required: Address the TODO in the Atlas Implementation
The TODO comment in src/inventoryWindows.ts (line 207) flags a potential architectural drawback where the atlas holds only rendered blocks instead of all possible item/block models. Our investigation shows:
- This atlas-related TODO is only present in this file.
- No GitHub issue exists to track this architectural concern.
Please either create a GitHub issue to track this enhancement or update/remove the comment if the behavior is now intentional.
User description
WIP TODO check 1.20.6 chest, test rp crash, fix shield, trident
INVESTIGATE doors and repeater
PR Type
Enhancement, Bug fix
Description
Added full support for rendering blocks in inventory GUI.
Implemented
generateGuiAtlasfor GUI texture generation.Enhanced
renderSlotto support GUI atlas-based rendering.Updated
mc-assetsdependency to version 0.2.41.Changes walkthrough 📝
guiRenderer.ts
Added GUI rendering and atlas generation logicrenderer/viewer/lib/guiRenderer.ts
generateGuiAtlasfor block and item rendering.getNonFullBlocksModelsfor resolving block models.worldrendererCommon.ts
Integrated GUI atlas generation into world rendererrenderer/viewer/lib/worldrendererCommon.ts
generateGuiAtlasinto asset update workflow.generateGuiTexturesmethod for GUI texture generation.inventoryWindows.ts
Enhanced inventory slot rendering with GUI atlassrc/inventoryWindows.ts
renderSlotto use GUI atlas for rendering.renderSlotExternalfunction.package.json
Updated mc-assets dependencypackage.json
mc-assetsdependency to version 0.2.41.pnpm-lock.yaml
Updated dependencies in lockfilepnpm-lock.yaml
mc-assetsto version 0.2.41 in lockfile.maxrects-packerdependency for texture packing.Summary by CodeRabbit
New Features
Chores
mc-assetsdependency to its latest compatible version for improved stability and performance.